Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-18383: Remove reserved.broker.max.id and broker.id.generation.enable #18478

Merged
merged 1 commit into from
Jan 29, 2025
Merged

KAFKA-18383: Remove reserved.broker.max.id and broker.id.generation.enable #18478

merged 1 commit into from
Jan 29, 2025

Conversation

FrankYang0529
Copy link
Member

The ZK mode is removed in 4.0, so we can remove related configuration as well.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@github-actions github-actions bot added core Kafka Broker performance storage Pull requests that target the storage module tiered-storage Related to the Tiered Storage feature labels Jan 10, 2025
chia7712
chia7712 reviewed Jan 18, 2025
View reviewed changes
core/src/main/java/kafka/log/remote/RemoteLogManager.java Outdated
@@ -376,7 +376,7 @@ RemoteStorageManager createRemoteStorageManager() {

private void configureRSM() {
final Map<String, Object> rsmProps = new HashMap<>(rlmConfig.remoteStorageManagerProps());
rsmProps.put(ServerConfigs.BROKER_ID_CONFIG, brokerId);
rsmProps.put(KRaftConfigs.NODE_ID_CONFIG, brokerId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remote storage is using broker.id rather than node.id. see

kafka/storage/src/test/java/org/apache/kafka/server/log/remote/storage/LocalTieredStorage.java

Line 238 in 78e3545

final Integer brokerIdInt = (Integer) configs.get(BROKER_ID);

maybe we should align the naming for this internal config. @showuon FYI

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @chia7712, I updated BROKER_ID to NODE_ID to fix CI error. Could you take a look when you have time? Thank you.

@FrankYang0529 FrankYang0529 force-pushed the KAFKA-18383 branch 5 times, most recently from 5b685dc to 273c29f Compare January 20, 2025 01:17
@chia7712
Copy link
Member

@FrankYang0529 please fix the conflicts

@chia7712 chia7712 mentioned this pull request Jan 20, 2025
Open
@chia7712
Copy link
Member

pending for the discussion https://github.com/apache/kafka/pull/18566/files#r1922782866

@FrankYang0529
Copy link
Member Author

Hi @chia7712, I resolved conflict and CI passes. Could you take a look when you have time? Thank you.

@github-actions github-actions bot added the small Small PRs label Jan 23, 2025
@FrankYang0529 FrankYang0529 changed the title KAFKA-18383: Remove reserved.broker.max.id, broker.id.generation.enable, and broker.id KAFKA-18383: Remove reserved.broker.max.id and broker.id.generation.enable Jan 23, 2025
@FrankYang0529 FrankYang0529 force-pushed the KAFKA-18383 branch 2 times, most recently from d39c027 to eda7730 Compare January 25, 2025 17:19
@chia7712
Copy link
Member

@FrankYang0529 could you please fix the conflicts?

@FrankYang0529
Copy link
Member Author

@chia7712 Resolved conflicts. Thanks.

@FrankYang0529 FrankYang0529 force-pushed the KAFKA-18383 branch 3 times, most recently from cdaf4f9 to b457780 Compare January 27, 2025 00:35
chia7712
chia7712 reviewed Jan 27, 2025
View reviewed changes
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FrankYang0529 thanks for this patch. Could you please also modify the unit test testAcceptsLargeNodeIdForRaftBasedCase? Maybe we can remove it directly.

docs/zk2kraft.html
@@ -86,7 +86,6 @@ <h5 class="anchor-heading">Removal Zookeeper configs</h5>
<ul>
<li><code>reserved.broker.max.id</code></li>
<li><code>broker.id.generation.enable</code></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please revise the description in line#84?

@chia7712 chia7712 merged commit 4dd0bcb into apache:trunk Jan 29, 2025
7 of 9 checks passed
chia7712 pushed a commit that referenced this pull request Jan 29, 2025
@FrankYang0529 @chia7712
KAFKA-18383 Remove reserved.broker.max.id and broker.id.generation.en…
ffb99a4 
…able (#18478)

Reviewers: Chia-Ping Tsai <[email protected]>
@FrankYang0529 FrankYang0529 deleted the KAFKA-18383 branch January 30, 2025 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chia7712 chia7712 chia7712 approved these changes

Assignees
No one assigned
Labels
core Kafka Broker performance small Small PRs storage Pull requests that target the storage module tiered-storage Related to the Tiered Storage feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@FrankYang0529 @chia7712